delete: preserve empty dirs in --backup-dir when deleting#967
Conversation
this demonstrates a subtle bug in PR 967
|
@x15sr71 thanks for the PR! There is a subtle bug with it in the handling of files that should not go into the backup dir. I've added another test to the PR that demonstrates the bug |
|
Thanks for the catch! Switched to |
|
Pushed a fix for the Solaris CI failure in the last CI run - Solaris returns |
|
@tridge The NetBSD failure is in daemon-access, the wo module was missing from the daemon listing, which is unrelated to this PR's delete/backup path. Both |
|
@tridge, pinging in case this got buried — the run results are no longer visible on the PR checks tab, so linking the only failed run from the last workflow run for reference: NetBSD run #394 Could you approve a workflow re-run when you get a chance? Thanks! P.S. Happy to rebase and squash the merge commits if you'd like — the history would just be your test commit and mine. |
| ok = do_rmdir_at(fbuf) == 0; | ||
| if (ok && make_backups > 0 && !(flags & DEL_FOR_BACKUP) && backup_dir) { | ||
| char *buf = get_backup_name(fbuf); | ||
| if (buf && do_mkdir_at(buf, ACCESSPERMS) == 0) { | ||
| if (INFO_GTE(BACKUP, 1)) |
There was a problem hiding this comment.
This removes fbuf before backup creation is known to succeed. If get_backup_name() returns NULL or do_mkdir_at() fails, the directory is gone but ok remains true and deletion reports success. ACCESSPERMS also loses the original directory mode. Please handle backup failure without reporting success and preserve the original attributes with failure and mode-preservation tests.
| # PR #967 makes delete_item() rename a directory into --backup-dir before | ||
| # falling back to rmdir, so empty dirs are preserved in the backup tree |
There was a problem hiding this comment.
This still describes the superseded rename implementation. Please update it to describe the protected-directory regression the final test guards.
Description
When using
--backup--backup-dir--delete, rsync was silently removing empty directories from the destination instead of preserving them in--backup-dir. Files in those directories were backed up correctly, but the empty directories themselves were not.Root cause
delete_item()routedS_ISDIRdirectly todo_rmdir_at(), bypassing themake_backups/backup_dirlogic that only existed in the non-directory else branch.Fix
After
delete_dir_contents()processes all children,delete_item()attempts rmdir()first — this is the atomic emptiness proof. If anything survived inside (e.g. a protect-filtered file),rmdir()fails and the directory is left untouched. Ifrmdir()succeeds,mkdir()recreates the directory in--backup-dir.EEXISTfrommkdir()is expected when child file backups already created the target directory.Also fixes
rmdir()error handling on Solaris, which returnsEEXIST(errno 17) instead ofENOTEMPTYfor non-empty directories — both are POSIX-valid. The existing check now handles both.Fixes #842.
Tests added in
testsuite/backup-empty-dir_test.py:backup-dirbackup-dir/sub/from child filebackups is not destroyed when parent dir is later backed up
--backup-dir— empty dir still deleted--delete-delayvariant — backup path reached for queued deletions